NE-2411: Add Template field to DNS operator#2765
NE-2411: Add Template field to DNS operator#2765grzpiotrowski wants to merge 2 commits intoopenshift:masterfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Skipping CI for Draft Pull Request. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Hello @grzpiotrowski! Some important instructions when contributing to openshift/api: |
openshift-ci
bot
added
the
size/L
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new exported feature gate variable 🚥 Pre-merge checks | ✅ 7 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (7 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/retitle [WIP] NE-2411: Add templates field to DNS operator |
openshift-ci
bot
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@grzpiotrowski: This pull request references NE-2411 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@operator/v1/types_dns.go`:
- Around line 528-535: The Zones slice lacks per-item format validation; update
the Zones field in operator/v1/types_dns.go to add a kubebuilder validation
Pattern that enforces RFC1123 DNS names or the literal ".". Specifically, add a
comment like //
+kubebuilder:validation:Pattern="^(\\.|[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*)$"
immediately above the Zones []string `json:"zones"` declaration so each entry is
validated as either "." or an RFC1123-compliant name. Ensure the annotation is
placed alongside the existing Required/MinItems tags.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 964b4669-8aa2-4d3f-bc4d-79f2a6924c53
📒 Files selected for processing (2)
features/features.gooperator/v1/types_dns.go
| // zones specifies the DNS zones this template applies to. | ||
| // Each zone must be a valid DNS name as defined in RFC 1123. | ||
| // The special zone "." matches all domains (catch-all). | ||
| // At least one zone must be specified. | ||
| // +kubebuilder:validation:Required | ||
| // +kubebuilder:validation:MinItems=1 | ||
| Zones []string `json:"zones"` | ||
|
|
There was a problem hiding this comment.
Add schema validation for zones item format.
The comments require RFC1123-compatible zones (plus "."), but the schema currently only enforces presence/count. That allows malformed zones to be persisted and fail downstream.
Suggested CRD validation guard
type Template struct {
// zones specifies the DNS zones this template applies to.
// Each zone must be a valid DNS name as defined in RFC 1123.
// The special zone "." matches all domains (catch-all).
// At least one zone must be specified.
+ // +kubebuilder:validation:items:Pattern=`^(\.|([a-z0-9]([-a-z0-9]*[a-z0-9])?)(\.([a-z0-9]([-a-z0-9]*[a-z0-9])?))*)$`
// +kubebuilder:validation:Required
// +kubebuilder:validation:MinItems=1
Zones []string `json:"zones"`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // zones specifies the DNS zones this template applies to. | |
| // Each zone must be a valid DNS name as defined in RFC 1123. | |
| // The special zone "." matches all domains (catch-all). | |
| // At least one zone must be specified. | |
| // +kubebuilder:validation:Required | |
| // +kubebuilder:validation:MinItems=1 | |
| Zones []string `json:"zones"` | |
| // zones specifies the DNS zones this template applies to. | |
| // Each zone must be a valid DNS name as defined in RFC 1123. | |
| // The special zone "." matches all domains (catch-all). | |
| // At least one zone must be specified. | |
| // +kubebuilder:validation:items:Pattern=`^(\.|([a-z0-9]([-a-z0-9]*[a-z0-9])?)(\.([a-z0-9]([-a-z0-9]*[a-z0-9])?))*)$` | |
| // +kubebuilder:validation:Required | |
| // +kubebuilder:validation:MinItems=1 | |
| Zones []string `json:"zones"` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@operator/v1/types_dns.go` around lines 528 - 535, The Zones slice lacks
per-item format validation; update the Zones field in operator/v1/types_dns.go
to add a kubebuilder validation Pattern that enforces RFC1123 DNS names or the
literal ".". Specifically, add a comment like //
+kubebuilder:validation:Pattern="^(\\.|[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*)$"
immediately above the Zones []string `json:"zones"` declaration so each entry is
validated as either "." or an RFC1123-compliant name. Ensure the annotation is
placed alongside the existing Required/MinItems tags.
grzpiotrowski
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
grzpiotrowski
changed the title
|
@grzpiotrowski: This pull request references NE-2411 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
operator/v1/types_dns.go
Outdated
| // actions defines a list of actions to apply to matching queries. | ||
| // | ||
| // +kubebuilder:validation:Required | ||
| // +kubebuilder:validation:MinItems=1 | ||
| // +required | ||
| Actions []TemplateAction `json:"actions"` |
There was a problem hiding this comment.
Just now I realized that with a slice we may have some duplicates which may be contradictory. Especially when the API will be enhanced to other actions (answer, authority, additional):
spec:
template:
actions:
- returnEmpty:
rcode: NOERRROR
- returnEmpty:
rcode: NXDOMAIN
There was a problem hiding this comment.
Changed the action to a struct.
So now the spec example would be:
spec:
template:
zones: ["example.com", "abc.com"]
queryType: AAAA
queryClass: IN
action:
rcode: NOERROR
|
@grzpiotrowski: This pull request references NE-2411 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
grzpiotrowski
force-pushed
the
NE-2411
branch
2 times, most recently
from
f3b5fcd to
cb219ec
Compare
|
@grzpiotrowski: This pull request references NE-2411 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
operator/v1/types_dns.go (1)
513-531:⚠️ Potential issue | 🟠 MajorValidate each
zonesentry against the documented DNS format.Right now the schema only checks count and length. Values like
bad_domainorexample.com.will be accepted here and only fail later when the CoreDNS config is rendered.Suggested schema guard
// +listType=set // +kubebuilder:validation:MinItems=1 // +kubebuilder:validation:MaxItems=15 // +kubebuilder:validation:items:MinLength=1 // +kubebuilder:validation:items:MaxLength=253 + // +kubebuilder:validation:items:Pattern=`^(\.|([a-z0-9]([-a-z0-9]*[a-z0-9])?)(\.([a-z0-9]([-a-z0-9]*[a-z0-9])?))*)$` // +required Zones []string `json:"zones,omitempty"`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operator/v1/types_dns.go` around lines 513 - 531, The Zones slice currently only validates count and length; add an items-level pattern validation so each entry must be either the single root string "." or a RFC1123-compliant DNS name (no trailing dot, labels 1–63 chars, letters/numbers/hyphen, labels cannot start or end with hyphen, full name max 253). Modify the Zones declaration in operator/v1/types_dns.go (the Zones []string field) to include a +kubebuilder:validation:items:Pattern tag that enforces "either '.' OR a valid DNS name" and update the field comment to note the stricter validation; this prevents invalid values like "bad_domain" or names with a trailing dot from being accepted.
🧹 Nitpick comments (1)
operator/v1/tests/dnses.operator.openshift.io/DNSTemplatePlugin.yaml (1)
4-5: Add a companion disabled-gate suite forspec.template.The suite-level
featureGatesfilter means this file only exercises CRDs whereDNSTemplatePluginis enabled (tests/types.go,tests/crd_filter.go). That leaves the disabled variants untested, so an accidental ungate/pruning regression would still pass.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operator/v1/tests/dnses.operator.openshift.io/DNSTemplatePlugin.yaml` around lines 4 - 5, Add a companion test suite that covers the DNSTemplatePlugin-disabled variant of spec.template so the CRD behavior when DNSTemplatePlugin is off is exercised; specifically, create a copy of the current DNSTemplatePlugin suite YAML but remove or clear the featureGates entry (so it does not list DNSTemplatePlugin) and save it as a separate suite (e.g., DNSTemplatePlugin.disabled.yaml), ensuring the new suite targets the same spec.template tests; this will let the existing tests/crd_filter.go suite-level featureGates filter run the disabled-case and catch gating/pruning regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@operator/v1/types_dns.go`:
- Around line 128-130: The CRD is missing RFC1123 pattern validation for the
zones slice; update the struct that defines the Zones field (Zones []string) to
include a kubebuilder validation Pattern annotation using the same DNS name
regex used elsewhere in this file (the regex around line 243) so each item is
RFC1123-compliant; keep the existing MinItems/MaxItems/MaxLength annotations and
add the kubebuilder:validation:Pattern line above the Zones field declaration to
enforce the per-item DNS name format.
---
Duplicate comments:
In `@operator/v1/types_dns.go`:
- Around line 513-531: The Zones slice currently only validates count and
length; add an items-level pattern validation so each entry must be either the
single root string "." or a RFC1123-compliant DNS name (no trailing dot, labels
1–63 chars, letters/numbers/hyphen, labels cannot start or end with hyphen, full
name max 253). Modify the Zones declaration in operator/v1/types_dns.go (the
Zones []string field) to include a +kubebuilder:validation:items:Pattern tag
that enforces "either '.' OR a valid DNS name" and update the field comment to
note the stricter validation; this prevents invalid values like "bad_domain" or
names with a trailing dot from being accepted.
---
Nitpick comments:
In `@operator/v1/tests/dnses.operator.openshift.io/DNSTemplatePlugin.yaml`:
- Around line 4-5: Add a companion test suite that covers the
DNSTemplatePlugin-disabled variant of spec.template so the CRD behavior when
DNSTemplatePlugin is off is exercised; specifically, create a copy of the
current DNSTemplatePlugin suite YAML but remove or clear the featureGates entry
(so it does not list DNSTemplatePlugin) and save it as a separate suite (e.g.,
DNSTemplatePlugin.disabled.yaml), ensuring the new suite targets the same
spec.template tests; this will let the existing tests/crd_filter.go suite-level
featureGates filter run the disabled-case and catch gating/pruning regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 95a71bcf-4a81-42a3-a7bb-55f27a83bf9b
⛔ Files ignored due to path filters (7)
openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**operator/v1/zz_generated.crd-manifests/0000_70_dns_00_dnses-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_70_dns_00_dnses-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_70_dns_00_dnses-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_70_dns_00_dnses-OKD.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_70_dns_00_dnses-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.featuregated-crd-manifests/dnses.operator.openshift.io/DNSTemplatePlugin.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**
📒 Files selected for processing (15)
features.mdfeatures/features.gooperator/v1/tests/dnses.operator.openshift.io/DNSTemplatePlugin.yamloperator/v1/types_dns.gooperator/v1/zz_generated.deepcopy.gooperator/v1/zz_generated.featuregated-crd-manifests.yamloperator/v1/zz_generated.swagger_doc_generated.gopayload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-TechPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-TechPreviewNoUpgrade.yaml
✅ Files skipped from review due to trivial changes (5)
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-DevPreviewNoUpgrade.yaml
- features.md
- payload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yaml
🚧 Files skipped from review as they are similar to previous changes (5)
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-TechPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-DevPreviewNoUpgrade.yaml
- operator/v1/zz_generated.featuregated-crd-manifests.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-TechPreviewNoUpgrade.yaml
- operator/v1/zz_generated.swagger_doc_generated.go
| // +optional | ||
| // +openshift:enable:FeatureGate=DNSTemplatePlugin | ||
| Template Template `json:"template,omitempty,omitzero"` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
gomod=$(fd -a '^go\.mod$' | head -n1)
echo "== toolchain =="
sed -n '/^go /p;/^toolchain /p' "$gomod"
echo
echo "== relevant dependency versions =="
sed -n '/k8s\.io\/apimachinery/p;/sigs\.k8s\.io\/json/p' "$gomod" || true
echo
tmpdir=$(mktemp -d)
python - "$gomod" "$tmpdir" <<'PY'
import io, pathlib, re, sys, urllib.request, zipfile
gomod_text = pathlib.Path(sys.argv[1]).read_text()
tmpdir = pathlib.Path(sys.argv[2])
downloaded = {}
def find_version(text, mod):
m = re.search(rf'^\s*{re.escape(mod)}\s+(v[^\s]+)', text, re.M)
return m.group(1) if m else None
def download(mod, ver):
if not ver or mod in downloaded:
return
data = urllib.request.urlopen(f"https://proxy.golang.org/{mod}/@v/{ver}.zip").read()
out = tmpdir / mod.replace("/", "_")
out.mkdir(parents=True, exist_ok=True)
with zipfile.ZipFile(io.BytesIO(data)) as zf:
zf.extractall(out)
downloaded[mod] = out
print(f"{mod} {ver} -> {out}")
api_ver = find_version(gomod_text, "k8s.io/apimachinery")
json_ver = find_version(gomod_text, "sigs.k8s.io/json")
download("k8s.io/apimachinery", api_ver)
if not json_ver and "k8s.io/apimachinery" in downloaded:
api_gomod = next(downloaded["k8s.io/apimachinery"].rglob("go.mod"), None)
if api_gomod:
json_ver = find_version(api_gomod.read_text(), "sigs.k8s.io/json")
download("sigs.k8s.io/json", json_ver)
PY
echo
echo "== omitzero support in downloaded modules =="
rg -n "omitzero" "$tmpdir" || true
echo
echo "== serializer JSON implementation references =="
rg -n "sigs\.k8s\.io/json|encoding/json" "$tmpdir" || trueRepository: openshift/api
Length of output: 2852
🏁 Script executed:
# First, locate and examine the types_dns.go file
fd -a 'types_dns\.go$' | head -n5Repository: openshift/api
Length of output: 142
🏁 Script executed:
# Examine the Template field and surrounding context (lines around 128-130)
sed -n '120,140p' operator/v1/types_dns.goRepository: openshift/api
Length of output: 1243
🏁 Script executed:
# Examine the Template struct definition (lines 482-564)
sed -n '480,570p' operator/v1/types_dns.goRepository: openshift/api
Length of output: 3254
🏁 Script executed:
# Search for existing omitzero usage in the codebase
rg -n 'omitzero' --type goRepository: openshift/api
Length of output: 50369
🏁 Script executed:
# Search for other optional value-type fields with similar patterns
rg -n 'json:"[^"]*,omitempty"' operator/v1/types_dns.go -A 1Repository: openshift/api
Length of output: 1977
🏁 Script executed:
# Check for tests related to Template field
fd -a '.*test.*\.go$' | xargs grep -l "Template" | head -5Repository: openshift/api
Length of output: 122
🏁 Script executed:
# Check if there are any GitHub issues or comments about omitzero handling
rg -n "omitzero" operator/v1/types_dns.go -B 2 -A 2Repository: openshift/api
Length of output: 293
🏁 Script executed:
# Verify zone validation specifically - check if there's a pattern constraint
sed -n '510,535p' operator/v1/types_dns.goRepository: openshift/api
Length of output: 1274
🏁 Script executed:
# Check if there's a pattern validation elsewhere for zones or DNS names
rg -n "pattern.*dns|pattern.*zone|kubebuilder:validation:Pattern" operator/v1/types_dns.goRepository: openshift/api
Length of output: 481
🏁 Script executed:
# Look for RFC 1123 validation patterns in the codebase
rg -n "RFC|1123|dns.*pattern" operator/v1/ --type go | head -20Repository: openshift/api
Length of output: 5596
🏁 Script executed:
# Check if structured merge diff or other serializers respect omitzero for nested structs
grep -r "omitzero" vendor/sigs.k8s.io/structured-merge-diff/v6/value/ -A 3 | head -30Repository: openshift/api
Length of output: 2618
Add pattern validation for zones to enforce RFC 1123 compliance.
The zones field documentation states entries must conform to RFC 1123, but the CRD schema only validates item length and count. Add a kubebuilder:validation:Pattern constraint matching the DNS name pattern used elsewhere in this file (e.g., line 243).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@operator/v1/types_dns.go` around lines 128 - 130, The CRD is missing RFC1123
pattern validation for the zones slice; update the struct that defines the Zones
field (Zones []string) to include a kubebuilder validation Pattern annotation
using the same DNS name regex used elsewhere in this file (the regex around line
243) so each item is RFC1123-compliant; keep the existing
MinItems/MaxItems/MaxLength annotations and add the
kubebuilder:validation:Pattern line above the Zones field declaration to enforce
the per-item DNS name format.
|
@grzpiotrowski: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
Add coreDNS template plugin API support to enable custom DNS query handling coreDNS. The main use case is filtering AAAA queries in IPv4-only clusters to reduce DNS latency and upstream load.
Example usage:
Intruducing new types:
Template- Template configuration with zones, query type, query class, and actionsQueryType- DNS query types (AAAA initially, the goal is to have it extensible to A, CNAME etc. in the future, same for other types)QueryClass- DNS query classes (only IN initially)ResponseCode- DNS response codes (NOERROR initially)TemplateAction- Discriminated union for template actionsReturnEmptyAction- initial action for returning empty DNS responses